Skip to content

Conversation

tdelafosse
Copy link

I introduced new "secure" renderers and printers that can be used in xwiki-platform to prevent XSS. See also the corresponding pull request made on xwiki-platform, that makes use of this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to guard against null value, it's possible to not use correctly the wiki syntax and therefore create arguments that dont have any value at all, should return false is that case.
bugsecureprinter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

I don't really understand why an attribute with a null value is a threat from a security point of view. In your example, it seems indeed that the syntax is wrongly used, but I don't think it will create a security break : all you will probably get is an html code looking like '<img src="/download/..." yeah="" )="" alt="example.jpg">'.
This isn't clean from an HTML point of view, but it is from a security point of view :)
It does not mean we shouldn't do something about that kind of things, but I don't think it's the right place or the right commit for it :). (But I must admit that isAttributeSafe would have been a better name for this method !).

Thanks !

Thomas

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Thomas, I am not saying it's a security issue, it's just a bug because the code should not throw a NPE in that particular case. Right now, without properly handling of that edge case, the user ends up with a system error on his screen.... So either the attributes need to be validated or cleaned before entering this method or the method should handle null values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sorry I misunderstood you. It should be OK now :)

@michitux
Copy link
Contributor

This has been implemented differently as part of XRENDERING-663/#212.

@michitux michitux closed this Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants