Item1931: NatSkin: XTHML Validation and SafeWikiPlugin issues
Priority: Normal
Current State: Closed
Released In:
Target Release: n/a
Applies To: Extension
Component: NatSkin
Branches:
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).
- 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
--
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!
--
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?
--
FranzJosefGigler - 25 Aug 2009
Obviously the need for relative urls were not recognized at that time.
--
MichaelDaum - 25 Aug 2009
Sorry for this rant
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:
- Forget about it and use a more liberal
SafeURIs
setting in SafeWikiPlugin (not appealing for me).
- Perhaps work on modifying SafeWikiPlugin to have more aggressive settings on a web-by-web basis (ugly).
- I could maintain my own set of hacks outside of Foswiki (painful).
- 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