Item1931: NatSkin: XTHML Validation and SafeWikiPlugin issues

pencil
Priority: Normal
Current State: Closed
Released In:
Target Release: n/a
Applies To: Extension
Component: NatSkin
Branches:
Reported By: PaulHarvey
Waiting For:
Last Change By: CrawfordCurrie
NatSkin doesn't produce very nice XHTML output (at least not from the point of view of the w3c validator).

I'm working on this from the point of view of being able to run SafeWikiPlugin + NatSkin.

Not only is SafeWikiPlugin stripping out some of the output that makes, eg. Edit mode totally broken, SafeWikiPlugin is actually barfing ungracefully on some of the mismatched/unclosed tags.

Steps to be done:

Move inline JavaScript to .js files in /pub.

the current skin structure allows to defined xxxjavascrit*tmpl and xxxstyles*tmpl files. as we have ADDTOHEAD now this structure can be abandoned now, afaics.
  • done for edit and history/rdiff/compare
  • topics with inline javascript:
    • attachjavascript.nat.tmpl
    • loginbase.nat.tmpl
    • oopslanguagechanged.nat.tmpl
    • renamejavascript.nat.tmpl
    • styles.nat.tmpl

Convert any JS that SafeWikiPlugin considers "complex"

(anything that isn't a straight function call) for on* events.

Convert usage of absolute URL macros that might result in protocol://hostname parts to URLPATH instead (if any).

  • done for USERACTIONS and WEBLINK. Note that there is no official api in Foswiki::Func to generate relative urls the same way we can do absolute urls. One would expect Foswiki::Func::getScriptUrlPath() to be similar to Foswiki::Func::getScriptUrl()=-. Alas as thats     not the case I replaced the Func calls in the plugins with =$session->getScriptUrl() which allows to do what we want.
  • Change DOCTYPE from XHTML 1.0/Strict to XHTML 1.0/Transitional (because it most certainly will be quite a lot of work to make it conform to Strict).

Work on "critical" validation errors (unclosed and mismatched tags).

  • can you list the ones that you've found?

Don't necessarily fix every validation error, as I suspect that some errors arise from supporting dodgy browsers (IE6, old Safari, etc).

  • yes that's mostly due to IE6 css hack in css. I don't really care of w3c barfing at those. It should barf at IE6 instead wink

-- PaulHarvey - 17 Aug 2009

Hi Paul. There've been some fixes in that direction already. Made inline comments above.

-- MichaelDaum - 17 Aug 2009

Thanks for the comments Michael, it may be that there aren't many unclosed/mismatched tags, but I do have lots of problems with my custom skin I've been basing on natskin. Probably time to turn that off and test natskin in isolation. Off the top of my head page.nat.tmpl has <meta> and <link> tags that aren't closed. Also Item1844 still applies (missing POST method).

-- PaulHarvey - 17 Aug 2009


Okay, so I have just got NatSkin working with SafeWikiPlugin. I probably need to do more testing on other templates besides edit and view.

Summary of changes:
  • templates/edit.nat.tmpl
    • Move inline <script> to pub/System/NatSkin/natskin.edit.js
  • =templates/styles.nat.tmpl
    • Moved ie6 png transparency <script></script> fragment to natskin.js
  • templates/foswiki.nat.tmpl
    • Added missing alt attribute
  • templates/topbar.nat.tmpl
    • Removed spurious <a> element
  • templates/view.nat.tmpl
    • Changed all <meta> and <link> to <meta/> and <link/>
  • templates/page.nat.tmpl
    • Changed DOCTYPE from Strict to Transitional
    • Changed all <meta> and <link> to <meta/> and <link/>
    • Added new <meta/> elements to hold NatSkin's PUBDIR and wiki guest name.
  • data/../NatSkinWebCreateNewTopicTemplate.txt
  • pub/../natskin.js
    • styles.nat.tmpl had some inline javascript to set a var called blankImg. Use the content of <meta name="foswiki.natSkin.pubDir" ...> instead.
  • pub/../Makefile
    • Add natskin.edit.js as a dependency.

... attached output of svn diff.

-- PaulHarvey - 20 Aug 2009

natedit.diff fixes some ordered list related validation issues. Also removed a %SUBST% around %PLUGINDESCRIPTIONS% which resulted in faulty non-validating output (truncated the ordered list so it never closes properly).

-- PaulHarvey - 20 Aug 2009

Okay merged your changes in. The new <meta ... /> tags weren't needed as all needed variables come via the foswiki object as initialized by the JQueryPlugin. Have a look at the other meta variables in your html and you'll find enuf stuff up there already.

-- MichaelDaum - 20 Aug 2009

Fixed in Item8250.

-- MichaelDaum - 20 Aug 2009

Hi Michael,

Thanks for merging those changes. However a few things didn't make it into your merge:

Index: NatSkin/data/System/NatSkinWebCreateNewTopicTemplate.txt
===================================================================
--- NatSkin/data/System/NatSkinWebCreateNewTopicTemplate.txt   (revision 4700)
+++ NatSkin/data/System/NatSkinWebCreateNewTopicTemplate.txt   (working copy)
@@ -89,7 +89,7 @@
 <!-- //formbuttons -->%TMPL:END%
 
 %TMPL:DEF{"formstart"}%<!-- formstart -->
-<form name="newTopicForm" id="newTopicForm" action="%SCRIPTURLPATH{edit}%/%BASEWEB%/">
+<form name="newTopicForm" id="newTopicForm" action="%SCRIPTURLPATH{edit}%/%BASEWEB%/" method="POST">
 <input type="hidden" name="t" value="%GMTIME{"$epoch"}%" />
 <input type="hidden" name="onlynewtopic" value="on" />
 <!-- //formstart -->%TMPL:END%
Index: NatSkin/templates/view.nat.tmpl
===================================================================
--- NatSkin/templates/view.nat.tmpl   (revision 4700)
+++ NatSkin/templates/view.nat.tmpl   (working copy)
@@ -13,7 +13,7 @@
 %TMPL:END%
 %TMPL:DEF{"metakeywords"}%%IF{
   "defined 'METAKEYWORDS'" 
-  then="<meta name='keywords' content='$percntMETAKEYWORDS$percnt'>$n" 
+  then="<meta name='keywords' content='$percntMETAKEYWORDS$percnt'/>$n" 
   else="$percntIF{\"'%WEB%.%TOPIC%'/Tag\" then=\"<meta name='keywords' content='$dollarpercntFORMFIELD{Tag}$dollarpercnt' />$n\"}$percnt"
 }%%TMPL:END%
 %TMPL:DEF{"metaauthor"}%%IF{

-- PaulHarvey - 21 Aug 2009

Also: Leaving in %RENDERHEAD% causes a few JS libs to load twice. I've removed it from my copy of page.nat.tmpl; perhaps we can consider removing %RENDERHEAD%?

The output of the view template in natskin is superb! smile

-- PaulHarvey - 21 Aug 2009

javascript.nat.tmpl introduced some more unclosed <meta> tags, and there's a spurious </div> in System/SiteBottomBar.txt.

Attaching a full diff with outstanding changes against NatSkin r4700.

-- PaulHarvey - 21 Aug 2009

Ah excellent. Applied all of your patch with the execption of the method="POST" to an edit form. That's not needed actually. Most of our edit links are getties.

-- MichaelDaum - 21 Aug 2009

Made a new release. Please have a try.

-- MichaelDaum - 21 Aug 2009

Hi Michael, it's all great except the NatEditPlugin changes haven't been applied yet.

Without these changes, apart from w3c validator complaining loudly - when SafeWikiPlugin is turned on, the edit page is a little strange at the bottom around the edittoolbar buttons.

Also SafeWikiPlugin seems to be disarming natskin.js, due to use of %PUBURL% in the plugin code. Attached a new diff for that problem.

-- PaulHarvey - 23 Aug 2009

True, these absolute urls are bad. Isn't there a config param for SafeWikiPlugin to allow certain absolute urls, i.e. the one to the own site?

-- MichaelDaum - 24 Aug 2009

Yes, one could use {SafeWikiPlugin}{SafeURIs} to make things more liberal. I think it's worth using URIs without the hostname part though, once we've made the switch - there shouldn't be any extra maintenance overhead?

-- PaulHarvey - 24 Aug 2009

Overhead => bad, that's a law of nature.

Unfortunately the Foswiki Func does deprecate part of the API that could be used by plugins to safely generate relative urls. For instance there is a Foswiki::Func::getScriptUrl() with all bells and whissles. The parallel Foswiki::Func::getScriptUrlPath() is rather crippled, being deprecated and you are recommended to use $Foswiki::cfg{ScriptUrlPath} instead.... which is not the same.

Foswiki::Func::getScriptUrl() directly calls a Foswiki::getScriptUrl() which then creates the correct link to the requested action using the SwitchBoard. Funnily enuf, Foswiki::getScriptUrl()'s first parameter servers as a switch to either create absolute or relative urls. Only the first is exported via Foswiki::Func while the latter would be much more usefull, i.e. given SafeWikiPlugin and page caching / proxying.

The story goes on with redirectto which does not work well with relative urls + url params and anchors. You will have to use absolute urls if you want to redirect to a location within the wiki including url params and anchors... another obstacle in getting away with absolute urls ...

There's a related config param to {SafeWikiPlugin}{SafeURIs} which is {PermitRedirectHostUrls}. Most probably you'd want to add all urls of the latter to the former which would reduce a bit the configuration overhead to get a wiki safe.

-- MichaelDaum - 25 Aug 2009

Hm, there must have been a good reason for deprecating Foswiki::Func::getScriptUrlPath(), otherwise we could dedeprecate, couldn't we? wink

-- FranzJosefGigler - 25 Aug 2009

Obviously the need for relative urls were not recognized at that time.

-- MichaelDaum - 25 Aug 2009

Sorry for this rant smile

Although I cannot enable SafeWikiPlugin on our production wiki yet, it is in my plans that we should be able to do so in the next couple of months. It seems anything public-facing on the internet is only getting more, not less traffic from bad guys hunting for exploits. TWiki-specific bot activity in our server logs is a constant concern, and I see SafeWikiPlugin as a potentially valuable tool to reduce the impact of exploits.

My question: is SafeWikiPlugin a serious tool for Foswiki? Or just a proof of concept, an experiment? It's unfortunate that I find myself defending our choice of TWiki/Foswiki when talking with colleagues and external collaborators who are in the business of software and security.

So, here are the options as I see it:
  1. Forget about it and use a more liberal SafeURIs setting in SafeWikiPlugin (not appealing for me).
  2. Perhaps work on modifying SafeWikiPlugin to have more aggressive settings on a web-by-web basis (ugly).
  3. I could maintain my own set of hacks outside of Foswiki (painful).
  4. Raise a new task against the API ( Foswiki::Func::getScriptUrlPath() ) to address issues Michael has noted (preferred).

-- PaulHarvey - 25 Aug 2009

Probably short-term is (1) to get things running for you. Long term should include (4), IMHO.

-- MichaelDaum - 25 Aug 2009

I think the core issue reported here has been resolved. The remaining discussion about relative vs absolute urls etc will be moved to a separate bug item.

-- MichaelDaum - 25 Aug 2009

getScriptUrlPath was deprecated because (1) it breaks URL rewriting and (2) it was never used. I have had a policy of deprecating APIs that are not used and break major blocks of functionality, as the alternative (continuing to support all the corner cases these APIs create, forever) is too horrible to contemplate.

-- CrawfordCurrie - 26 Aug 2009

ItemTemplate edit

Summary NatSkin: XTHML Validation and SafeWikiPlugin issues
ReportedBy PaulHarvey
Codebase trunk
SVN Range Foswiki-1.0.0, Thu, 08 Jan 2009, build 1878
AppliesTo Extension
Component NatSkin
Priority Normal
CurrentState Closed
WaitingFor
Checkins NatSkin:4cbf0fdddb8d NatSkin:93a6168d9aaf
TargetRelease n/a
ReleasedIn
I Attachment Action Size Date Who Comment
natedit.diffdiff natedit.diff manage 3 K 20 Aug 2009 - 07:53 PaulHarvey  
natskin.diffdiff natskin.diff manage 16 K 20 Aug 2009 - 03:30 PaulHarvey  
natskin4700.diffdiff natskin4700.diff manage 3 K 21 Aug 2009 - 06:57 PaulHarvey  
natskinplugin4701.diffdiff natskinplugin4701.diff manage 1 K 23 Aug 2009 - 12:32 PaulHarvey  
Topic revision: r23 - 26 Aug 2009, CrawfordCurrie
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy