Move authoring UI inline JS handlers to data attributes - #168
Conversation
The editor screens built handler arguments by interpolating template values directly into inline onclick/onchange attributes. Those values now travel in data-* attributes and are read back through delegated listeners, which matches how the rest of the UI binds behaviour and keeps markup and data separate. Writes that placed those values into the DOM now use text APIs rather than html(), and ThemeDataServlet escapes its JSON output so theme metadata cannot produce a malformed document. Adds AuthoringUiSinkAuditTest to keep the editor templates on this pattern.
The media gallery rendered the media file name into single-quoted HTML attributes; those attributes are now double-quoted so the value stays data. The sink-audit test description now states the rule it enforces, and the repeated per-page comment is collapsed to one line. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
mraible
left a comment
mraible
left a comment
There was a problem hiding this comment.
The .html() → .text() swaps and the quote flips are the right direction, but the same sink family is still open in a few places the new audit doesn't reach, so I don't think this closes the authoring-UI XSS class yet. Inline comments for the in-diff ones; three are outside the diff:
Bookmarks.jsp:134still emits<a href='<s:property value="#bookmark.url" />'>.escapeHtml4leaves'alone, so a bookmark URL likex' onmouseover='alert(1)breaks out of the attribute (stored XSS against every member who opens the Bookmarks page). Same pattern this PR fixed inMediaFileView.jsp/MediaFileImageChooser.jsp; flip it to double quotes here too.Comments.jsp:279has the same single-quotedhref='<s:property value="#comment.url" />'with a commenter-supplied URL, which is stored XSS against whoever moderates comments. Either include it here or drop the "authoring UI sinks are closed" framing from the description.CreateWeblog.jsp:118still does$('#themedescription').html(data.description)from the sameThemeDataServletresponse thatThemeEdit.jspnow writes with.text().
| function confirmDeleteFolder() { | ||
| $('#boomarks_delete_folder_folderId').val($('#bookmarks_folderId:first').val()); | ||
| $('#deleteBlogrollName').html('<s:property value="%{folder.name}"/>'); | ||
| $('#deleteBlogrollName').text('<s:property value="%{folder.name}"/>'); |
There was a problem hiding this comment.
<s:property> HTML-escapes with escapeHtml4, which doesn't touch ', so a folder named Matt's Links renders as .text('Matt's Links') and the SyntaxError takes out every function in this <script> block (delete/rename buttons stop working). And now that it's .text(), Tom & Jerry displays as Tom & Jerry. Use <s:property value="%{folder.name}" escapeJavaScript="true" escapeHtml="false"/> here, or read the name from a data- attribute on the trigger element instead of inlining it in JS.
| pw.print("\", "); | ||
| pw.print("\"description\" : \""); | ||
| pw.print(theme.getDescription()); | ||
| pw.print(StringEscapeUtils.escapeJson(theme.getDescription())); |
There was a problem hiding this comment.
CreateWeblog.jsp:118 consumes this same field with $('#themedescription').html(data.description), so a theme description is inert on Theme Edit but still rendered as markup on Create Weblog. Worth switching that call to .text() in this PR, since the audit test only scans jsps/editor and won't notice it.
| $('#category-name').html(name); | ||
| $('#category-name').text(name); | ||
| if ( inUse ) { | ||
| $('#category-in-use').css('display','block'); |
There was a problem hiding this comment.
Pre-existing, but since this function is being reworked: lines 294/297 toggle #category-emtpy while the element is #category-empty (line 266), so the "no entries in this category" message never shows.
| url: "<s:url value='themedata'/>", | ||
| data: {theme: themeId}, success: function (data) { | ||
| $('#themeDescription').html(data.description); | ||
| $('#themeDescription').text(data.description); |
There was a problem hiding this comment.
Behaviour change worth a line in the description: a shared theme whose theme.xml <description> carries markup now renders it as literal text. The bundled themes are plain text, so I think that's fine, just disclose it.
| public class AuthoringUiSinkAuditTest { | ||
|
|
||
| private static final Path EDITOR_JSP_DIR = | ||
| Paths.get("src", "main", "webapp", "WEB-INF", "jsps", "editor"); |
There was a problem hiding this comment.
Two things here. This is cwd-relative, so it only runs from app/; surefire already sets project.build.directory for this module (see ApplicationResourcesTest), so deriving the path from that (or basedir) lets it run from an IDE rooted at the repo.
More importantly, the patterns give false assurance: SCRIPT_VAR_LITERAL only matches var x = '<s:property, HANDLER_LITERAL only matches a double-quoted handler attribute with a single-quoted inner literal, and HTML_WRITE ignores .append/innerHTML/.attr('src'). A single-quoted <s:property> as a call argument (.text('<s:property .../>'), previewImage('<s:property .../>')) or a single-quoted attribute (href='<s:property .../>') isn't caught, and both remaining Bookmarks.jsp sinks pass this test green.
|
|
||
| <%-- Source data for the "move entries to" select, carried as escaped | ||
| attributes rather than generated JavaScript literals. --%> | ||
| <div id="category-option-data" style="display:none"> |
There was a problem hiding this comment.
Nit: the .category-edit-link anchors above already carry data-category-id / data-category-name for every category, so populateCategorySelect could read those instead of maintaining a second serialised copy.
| } | ||
| } | ||
|
|
||
| private static String flatten(String source) { |
There was a problem hiding this comment.
Nit: HANDLER_LITERAL uses \s* and [^"]*, both of which already span newlines, so flatten() / flattenSource don't change any match and can go.
2 participants
Several authoring pages embed values in inline JavaScript string literals and
then write them into the page with jQuery
.html(). This refactor moves thosevalues into data attributes and writes them through text APIs, so JSPs stop
hand-concatenating markup and data.
What changed
into double-quoted, HTML-escaped
data-*attributes.textContent,.text(),.val(), or DOM constructors..html()writes with a text API.entry lists, and entry editing, including the category iterator and
delete-modal cases.
ThemeDataServletreturn validJSON.
Tests
and
</script>remain text through each path.author or administrator session.
single-quoted attribute, or a
.html()sink.