snoopdave commented on code in PR #221:
URL: https://github.com/apache/roller/pull/221#discussion_r4419665733
##########
app/src/main/webapp/WEB-INF/velocity/weblog.vm:
##########
@@ -479,21 +479,21 @@ Show bookmarks as a HTML ul list.
<li class="rFolderItem">
#if($utils.isNotEmpty($bookmark.Image))
#if( $bookmark.Image.startsWith("http://";) )
- <a href="$bookmark.url">
+ <a href="$utils.escapeHTML($bookmark.url)">
<img alt="$bookmark.description" border="0"
- src="$bookmark.image" class="rBookmark" /></a>
+ src="$utils.escapeHTML($bookmark.image)" class="rBookmark"
/></a>
#else
- <a href="$bookmark.url">
+ <a href="$utils.escapeHTML($bookmark.url)">
<img alt="$bookmark.description" border="0"
- src="$bookmark.image" class="rBookmark" /></a>
+ src="$utils.escapeHTML($bookmark.image)" class="rBookmark"
/></a>
#end
#else
#if($utils.isNotEmpty( $bookmark.FeedUrl))
- <a href="$bookmark.feedUrl"><img class="smrssbadge"
+ <a href="$utils.escapeHTML($bookmark.feedUrl)"><img
class="smrssbadge"
src="$url.absoluteSite/images/smrssbadge.gif"
alt="$text.get("macro.bookmark.urlFeed")"/></a>
#end
- <a href="$bookmark.url"
title="$bookmark.description">$bookmark.name</a>
+ <a href="$utils.escapeHTML($bookmark.url)"
title="$bookmark.description">$bookmark.name</a>
Review Comment:
🤖Claude: fixed by escaping `$bookmark.name` and `$bookmark.description` with
`$utils.escapeHTML` in the blogroll macro, in the link text, `title` and `alt`
(18b432d0a). CHANGES.md notes it.
##########
app/src/main/webapp/WEB-INF/velocity/weblog.vm:
##########
@@ -479,21 +479,21 @@ Show bookmarks as a HTML ul list.
<li class="rFolderItem">
#if($utils.isNotEmpty($bookmark.Image))
#if( $bookmark.Image.startsWith("http://";) )
Review Comment:
🤖Claude: fixed by removing the `#if` and keeping one branch (18b432d0a).
##########
app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogBookmarkWrapper.java:
##########
@@ -59,20 +67,37 @@ public String getDescription() {
public String getUrl() {
- return this.pojo.getUrl();
+ return linkOrEmpty(this.pojo.getUrl());
}
public Integer getPriority() {
return this.pojo.getPriority();
}
public String getImage() {
- return this.pojo.getImage();
+ return linkOrEmpty(this.pojo.getImage());
}
public String getFeedUrl() {
- return this.pojo.getFeedUrl();
+ return linkOrEmpty(this.pojo.getFeedUrl());
+ }
+
+ /**
+ * Returns the link if it is relative or uses a scheme a blogroll link
+ * needs (http, https, ftp, mailto), else an empty string. Browsers ignore
tabs and
+ * line breaks inside a scheme, so those are removed before the check.
+ */
+ static String linkOrEmpty(String link) {
Review Comment:
🤖Claude: fixed by adding `LinkScheme.of`, which checks the part before the
first `/`, `?` or `#`: a `&` there, or a scheme that is not valid, makes the
link unreadable and the wrapper returns `""`. Otherwise the scheme must be in
`ALLOWED_SCHEMES` (18b432d0a).
##########
app/src/test/java/org/apache/roller/weblogger/pojos/wrapper/WeblogBookmarkWrapperTest.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.pojos.wrapper;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+/**
+ * Blogroll links reach templates only when they are relative or use a scheme
+ * a blogroll link needs.
+ */
+class WeblogBookmarkWrapperTest {
+
+ @Test
+ void keepsWebAndRelativeLinks() {
+ assertEquals("https://example.com/a";,
WeblogBookmarkWrapper.linkOrEmpty("https://example.com/a";));
+ assertEquals("HTTP://example.com/",
WeblogBookmarkWrapper.linkOrEmpty("HTTP://example.com/"));
+ assertEquals("ftp://example.com/f";,
WeblogBookmarkWrapper.linkOrEmpty("ftp://example.com/f";));
+ assertEquals("mailto:[email protected]";,
WeblogBookmarkWrapper.linkOrEmpty("mailto:[email protected]";));
+ assertEquals("/blog/page",
WeblogBookmarkWrapper.linkOrEmpty("/blog/page"));
+ assertEquals("page?a=b:c",
WeblogBookmarkWrapper.linkOrEmpty("page?a=b:c"));
+ assertNull(WeblogBookmarkWrapper.linkOrEmpty(null));
+ }
+
+ @Test
+ void dropsOtherSchemes() {
Review Comment:
🤖Claude: fixed by adding the three forms to `WeblogBookmarkWrapperTest`,
plus a query with `&` after the path that is kept, and a new `LinkSchemeTest`
(18b432d0a).
##########
app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java:
##########
@@ -270,6 +285,26 @@ private void importOpmlElement(
}
}
+ /**
+ * Returns the value if it is an absolute http or https URL, else null.
+ */
+ static String webLinkOrNull(String value) {
Review Comment:
🤖Claude: fixed by making `webLinkOrNull` use `LinkScheme.of`: it keeps a
link whose scheme is `http` or `https` and that has a host after `//`.
`BookmarkImportParsingTest` now imports a link with a space and one with `_` in
the host, and still skips one with no host. The CHANGES.md caveat is gone
(18b432d0a).
--
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]
