snoopdave commented on code in PR #221:
URL: https://github.com/apache/roller/pull/221#discussion_r4419635343
##########
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:
The scheme check only sees a scheme written literally, so a link with a
character reference before the path (for example `x:y`, `x:y` or
`x:y`) is treated as relative. That's fine in `weblog.vm`, which escapes,
but not in a custom template that prints `$bookmark.url` directly.
Suggest checking the part before the first `/`, `?` or `#`:
- if it contains `:`, the scheme must be in `ALLOWED_SCHEMES`;
- if it contains `&`, return `""`.
Real relative links don't have `&` before the path. (Decoding with
`unescapeHtml4` isn't enough: it doesn't know HTML5 names like `:`.)
##########
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:
Minor: both branches of this `#if` are now identical, so it can go.
##########
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:
If the wrapper change above goes in, add the three character-reference forms
here.
##########
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:
Could we escape the name and description the same way as the URLs?
`$bookmark.name` (link text) and `$bookmark.description` (`title` here, `alt`
above) are still printed as stored, and import copies them straight from the
OPML:
```velocity
title="$utils.escapeHTML($bookmark.description)">$utils.escapeHTML($bookmark.name)</a>
```
##########
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:
The full `java.net.URI` parse drops real blogroll links that browsers accept
(`_` in a host, unencoded `|`), and doesn't add much once the scheme is checked
and the output is escaped. Could import use the same scheme rule as
`WeblogBookmarkWrapper.linkOrEmpty`, with only `http`/`https` allowed? One rule
for both paths, fewer good links lost, and the `CHANGES.md` caveat goes away.
--
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]
