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.
--
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]
