On Thu, Jul 25, 2019 at 5:33 PM Simon Urli <[email protected]> wrote:
> Hi Marius, > > On 25/07/2019 16:22, Marius Dumitru Florea wrote: > > On Thu, Jul 25, 2019 at 11:39 AM Simon Urli <[email protected]> > wrote: > > > >> Hi everyone, > >> > >> I'm currently working on improving security on XWiki comments. We > >> already use a restricted mode in our comments but that does not cover > >> every possible case. In order to improve it we should also filter out > >> some part of the html when using the html macro. > >> > > > > Is the HTML macro needed in comments? Do you have a real use case for > this? > > Wouldn't it be more simple to completly forbid HTML and script macros in > > comments? > > That's also an option, but potentially more breaking. > > IMO the decision was already taken a while ago with > > https://github.com/xwiki-contrib/syntax-markdown/commit/907dc0630a1b7cad4292c1472deb7ae4d68c66ae I don't understand why a change in markdown syntax (contrib project) can be considered as decision for the XWiki Project. But anyway, any decision can be revisited. Again, do you have a real use case where you need HTML macro in comments? > > which forces to use html macro with clean option when used in restricted > mode, but does not prevent to use html macro at all. Contrary to the > script macro which have all been disabled on the following commit: > > https://github.com/xwiki/xwiki-platform/commit/544d290a37cf86dff27aa7bc12be3f6a219f1508 > > Simon > > > > Thanks, > > Marius > > > > > >> > >> I propose: > >> > >> (a) that we use a configurable whitelist of HTML attributes that > >> would be allowed in the output HTML: all the other attributes would be > >> filtered out. > >> > >> (b) that the HTML macro is put in restricted mode for users who do > >> not have scripting rights. > >> > >> For (a) I'm hesitating between a whitelist or a blacklist: I assume a > >> blacklist would be shorter but there's also more risk of missing > >> something. On the contrary a configurable whitelist doesn't prevent > >> administrator to accept more than what we give in standard. > >> > >> A first whitelist could be (taken from: > >> > >> > https://github.com/xwiki/xwiki-platform/pull/122/files#diff-c33fcb5dca86b155928768dd6e6fbf7eR146 > >> ) > >> alt, class, height, id, name, rel, scope, style, target, title, width > >> > >> Note that href is not included in this list for example. > >> > >> WDYT? > >> > >> Simon > >> > >> -- > >> Simon Urli > >> Software Engineer at XWiki SAS > >> [email protected] > >> More about us at http://www.xwiki.com > >> > > -- > Simon Urli > Software Engineer at XWiki SAS > [email protected] > More about us at http://www.xwiki.com >
