mraible commented on code in PR #168:
URL: https://github.com/apache/roller/pull/168#discussion_r3891438824
##########
app/src/main/webapp/WEB-INF/jsps/editor/Bookmarks.jsp:
##########
@@ -338,7 +338,7 @@ We used to call them Bookmarks and Folders, now we call
them Blogroll links and
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}"/>');
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/ajax/ThemeDataServlet.java:
##########
@@ -80,17 +81,21 @@ public void doGet(
}
for (Iterator<SharedTheme> it = themes.iterator(); it.hasNext();) {
SharedTheme theme = it.next();
+ // Theme metadata comes from theme.xml, which an operator can edit
+ // or install; escape it so a quote or newline cannot break out of
+ // the string and produce malformed JSON.
pw.print(" { \"id\" : \"");
- pw.print(theme.getId());
+ pw.print(StringEscapeUtils.escapeJson(theme.getId()));
pw.print("\", ");
pw.print("\"name\" : \"");
- pw.print(theme.getName());
+ pw.print(StringEscapeUtils.escapeJson(theme.getName()));
pw.print("\", ");
pw.print("\"description\" : \"");
- pw.print(theme.getDescription());
+ pw.print(StringEscapeUtils.escapeJson(theme.getDescription()));
Review 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.
##########
app/src/test/java/org/apache/roller/weblogger/ui/struts2/editor/AuthoringUiSinkAuditTest.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License. For additional
+ * information regarding copyright in this work, please see the NOTICE
+ * file in the top level directory of this distribution.
+ */
+package org.apache.roller.weblogger.ui.struts2.editor;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+/**
+ * Structural audit of the authoring UI templates.
+ *
+ * <p>Values that originate from weblog content are rendered by the editor
JSPs.
+ * This test enforces the two structural rules that keep those values inert:
they
+ * travel in double-quoted <code>data-*</code> attributes rather than inline
+ * handler literals, and they are written to the DOM through a text API. It is
a
+ * source audit rather than a behavioural test because the guarantee is a
+ * property of the whole page family, not of any one code path.
+ */
+public class AuthoringUiSinkAuditTest {
+
+ private static final Path EDITOR_JSP_DIR =
+ Paths.get("src", "main", "webapp", "WEB-INF", "jsps", "editor");
Review 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.
##########
app/src/main/webapp/WEB-INF/jsps/editor/Categories.jsp:
##########
@@ -288,7 +288,7 @@
function showCategoryDeleteModal( id, name, inUse ) {
$('#categoryRemove_removeId').val(id);
$('#categoryEdit_bean_name').val(name);
- $('#category-name').html(name);
+ $('#category-name').text(name);
if ( inUse ) {
$('#category-in-use').css('display','block');
Review 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.
##########
app/src/main/webapp/WEB-INF/jsps/editor/ThemeEdit.jsp:
##########
@@ -246,7 +246,7 @@
$.ajax({
url: "<s:url value='themedata'/>",
data: {theme: themeId}, success: function (data) {
- $('#themeDescription').html(data.description);
+ $('#themeDescription').text(data.description);
Review 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.
##########
app/src/main/webapp/WEB-INF/jsps/editor/Categories.jsp:
##########
@@ -319,4 +320,30 @@
});
}
+ // Values come from data-* attributes and are bound via delegated
listeners.
+ $(document).on('click', '.category-edit-link', function (event) {
+ event.preventDefault();
+ showCategoryEditModal($(this).attr('data-category-id'),
+ $(this).attr('data-category-name'),
+ $(this).attr('data-category-desc'),
+ $(this).attr('data-category-image'));
+ });
+
+ $(document).on('click', '.category-delete-link', function (event) {
+ event.preventDefault();
+ showCategoryDeleteModal($(this).attr('data-category-id'),
+ $(this).attr('data-category-name'),
+ $(this).attr('data-category-in-use') === 'true');
+ });
+
</script>
+
+<%-- 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">
Review 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.
##########
app/src/test/java/org/apache/roller/weblogger/ui/struts2/editor/AuthoringUiSinkAuditTest.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License. For additional
+ * information regarding copyright in this work, please see the NOTICE
+ * file in the top level directory of this distribution.
+ */
+package org.apache.roller.weblogger.ui.struts2.editor;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+/**
+ * Structural audit of the authoring UI templates.
+ *
+ * <p>Values that originate from weblog content are rendered by the editor
JSPs.
+ * This test enforces the two structural rules that keep those values inert:
they
+ * travel in double-quoted <code>data-*</code> attributes rather than inline
+ * handler literals, and they are written to the DOM through a text API. It is
a
+ * source audit rather than a behavioural test because the guarantee is a
+ * property of the whole page family, not of any one code path.
+ */
+public class AuthoringUiSinkAuditTest {
+
+ private static final Path EDITOR_JSP_DIR =
+ Paths.get("src", "main", "webapp", "WEB-INF", "jsps", "editor");
+
+ /**
+ * An inline event handler attribute whose body opens a single-quoted
+ * JavaScript string containing a Struts property. Newlines are collapsed
+ * before matching because these handlers routinely wrap across lines.
+ */
+ private static final Pattern HANDLER_LITERAL =
+ Pattern.compile("on[a-zA-Z]+\\s*=\\s*\"[^\"]*'\\s*<s:property");
+
+ /** A bare JavaScript variable assignment from a Struts property. */
+ private static final Pattern SCRIPT_VAR_LITERAL =
+ Pattern.compile("var\\s+\\w+\\s*=\\s*'\\s*<s:property");
+
+ /**
+ * A jQuery html() write. Calls passing only a localized string, an empty
+ * string, or nothing are inert and are excluded.
+ */
+ private static final Pattern HTML_WRITE =
+
Pattern.compile("\\.html\\(\\s*(?!\\)|''|\"\"|'<s:text|\"<s:text)[^)]");
+
+ /**
+ * The comment moderation screen round-trips already-encoded comment markup
+ * through html(); the settled fix plan treats that as separate follow-up
+ * hardening rather than part of this sink family.
+ */
+ private static final Set<String> EXCLUDED_FILES =
+ new HashSet<>(Arrays.asList("Comments.jsp"));
+
+ private List<Path> editorJsps() throws IOException {
+ Path dir = EDITOR_JSP_DIR;
+ assertTrue(Files.isDirectory(dir), "cannot locate editor JSPs at "
+ + dir.toAbsolutePath() + " (run from the app module)");
+ try (Stream<Path> files = Files.list(dir)) {
+ return files.filter(p ->
p.getFileName().toString().endsWith(".jsp"))
+ .filter(p ->
!EXCLUDED_FILES.contains(p.getFileName().toString()))
+ .sorted()
+ .collect(Collectors.toList());
+ }
+ }
+
+ private static String flatten(String source) {
Review Comment:
Nit: `HANDLER_LITERAL` uses `\s*` and `[^"]*`, both of which already span
newlines, so `flatten()` / `flattenSource` don't change any match and can go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
