Hello devs,
Disclaimer: the research and the drafting of this proposal were done by Claude Code, on my prompting. Each point was checked against the code on master, but please verify and push back where something is wrong or incomplete.
I’d like to propose adding a few best practices to the Security Developer Guide. They are all about using existing APIs correctly, and none requires a code change. Below are the proposed changes to the page source, as diffs. Most extend an existing section; one is a new section.
1. Scripting and Escaping: pick the escaping for the exact output context
* ##$services.rendering.escape($content, 'xwiki/2.1')## for XWiki syntax
* ##$escapetool.xml($content)## for HTML output. This can also be used in an HTML macro and escapes ##{##, thereby preventing the closing of the HTML macro through user input.
+Use the escaping that matches the exact place where the value ends up:
+
+* In an HTML attribute, use ##$escapetool.xml##, not ##$escapetool.html##, which doesn't escape the single quote.
+* In a JavaScript string, use ##$escapetool.javascript## (or ##$escapetool.json## to produce a JSON value). ##$escapetool.xml## doesn't escape the backslash, so it's not enough there. Don't insert values in JavaScript template literals: no escaping tool supports them.
+* In a query, don't use ##$escapetool.sql##: bind the value instead (see [[Queries>>||anchor="HQueries"]]).
+* Never evaluate a value that a user can control as Velocity (e.g., with ###evaluate##): no escaping makes it safe.
+
Make sure you always test if escaping actually protects against attacks by writing appropriate tests.
2. HTML: the HTML macro’s cleaning doesn’t replace escaping
... See also the section on [[HTTP requests and redirects>>||anchor="#HHTTPRequestsandRedirects"]] for extra precautions that you should take if the user doesn't expect to land on a different domain when clicking on a link/button, like a "Cancel" button.
+
+The cleaning done by the HTML macro (##clean="true"##, the default) is not a replacement for escaping. When the author of the content has script right, which is always the case for HTML produced by a Velocity script, the cleaning only makes the HTML valid: it doesn't remove scripts or event handlers. So always escape the values you insert in HTML, even inside an HTML macro.
3. Protect against XXE attacks: use the XML Module helpers
Always follow the [[OWASP recommendations to protect against XXE attacks>>https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html]] when parsing XML.
+
+In practice, don't create XML parsers (##XMLInputFactory##, ##DocumentBuilderFactory##, ##SAXParserFactory##, etc.) yourself: use the helpers of the [[XML Module>>extensions:Extension.XML Module]], ##StAXUtils.getXMLStreamReader(...)## or ##XMLUtils.parse(...)##, which are already configured not to load external DTDs and entities. If you really need your own parser, disable DTDs and external entities explicitly: enabling ##XMLConstants.FEATURE_SECURE_PROCESSING## is not enough with every parser implementation.
4. Right Checks in Script Services: pass the entity, and programming right on subwikis
If the script service exposes information or executes actions without further right checks, it must check for programming right of the context author.
+Always pass the entity on which you check a right, instead of relying on the default:
+
+* Without an entity, ##hasAccess(Right.ADMIN)## (as well as ##$xwiki.hasAdminRights()## and ##$hasAdmin##) checks the right on the current document, so it's also true for someone who only administers the space of the current page. To check wiki administration, pass the wiki: ##hasAccess(Right.ADMIN, new WikiReference(wikiId))##, or use ##$xwiki.hasWikiAdminRights()##.
+* For script and programming rights, the entity decides whose rights are checked: ##hasAccess(Right.SCRIPT, reference)## checks the content author of the document at ##reference##, not the author of the running script. To check the author of the running script, don't pass an entity. To check a given user on a given entity, use ##AuthorizationManager#hasAccess(right, user, entity)##.
+
+Programming right can only be granted on the main wiki: an administrator of a subwiki has script right but never programming right. Don't require programming right for something that subwiki administrators should be able to do, and do require it for anything they must not be able to do.
+
Note that context author rights are currently not consistently enforced in XWiki, in particular there is no such concept in JavaScript. This is an area for future improvements, new code should still take context author rights into account.
5. New section, after “Returning Data in Script Services”: Saving Documents in Script Services
When returning any object in a script service, ensure that all its methods properly check access rights and don't allow modifying data without proper access right checks. Use wrapper objects to add right checks or hide dangerous methods. For example, returning an ##XWikiDocument## is not safe as it allows modifying author information and executing the content with the new author.
+== Saving Documents in Script Services ==
+
+Script services, and any other API that scripts can call, must never save a document with the current user as author when the author of the calling script doesn't have programming right. Otherwise, a script that a user with more rights merely views saves content in that user's name, and that content then executes with that user's rights. The public ##Document#save()## API already behaves this way: without programming right, it saves the document with the script's author as author. Reuse it, or apply the same check.
+
== Executing Code or XWiki Syntax ==
6. Executing Code or XWiki Syntax: display properties, don’t parse their raw value
* Execute the code or transformations with the correct author in context. In Java, ##AuthorExecutor## should be used for this. There is no way to do this in Velocity. There are hacks like ##dropPermissions## but they are prone to security vulnerabilities and should thus be avoided.
+
+To render an XObject property, display it, e.g., with ##$doc.display('property', $object)##: it's then executed with the rights of the effective metadata author of its document. Never insert the raw value of a property (##$object.getValue('property')##) in content that is parsed as XWiki syntax or evaluated as Velocity, since it would then be executed with the rights of the author of the page doing the rendering. Note that, despite its name, ##$object.get('property')## returns the displayed property and not its value: use ##getValue## to read or compare the value.
If accepted
I’ll apply these changes to the page, and mirror them in the XWiki LLM knowledge base (xwiki-dev-llm), pointing to the guide as the source of truth.
WDYT?
Thanks