snoopdave commented on code in PR #190:
URL: https://github.com/apache/roller/pull/190#discussion_r4040899150
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -80,7 +80,12 @@ public class Utilities {
private static final Pattern CLOSING_A_TAG_PATTERN = Pattern.compile(
"</a>", Pattern.CASE_INSENSITIVE);
private static final Pattern OPENING_A_TAG_PATTERN = Pattern.compile(
- "<a href=.*?>", Pattern.CASE_INSENSITIVE);
+ "<a\\s+href\\s*=.*?>", Pattern.CASE_INSENSITIVE |
Pattern.DOTALL);
+ private static final Pattern A_HREF_PATTERN = Pattern.compile(
+
"<a\\s+href\\s*=\\s*(?:\"([^\"]*)\"|'([^']*)'|([^\\s\"'=<>`]+))\\s*>",
Review Comment:
🐞Claude Issue: **Important:** The unquoted-value alternative `[^\s"'=<>`]+`
excludes `=`, so any unquoted `href` carrying a query string stops at the first
`=` and the whole match fails — the link is dropped:
```
<a href=https://ex.com/?a=1>x</a> -> <a>x</a>
<a href=https://ex.com/?a=1&b=2>x</a> -> <a>x</a>
```
Both render as links on `master`, so this is a regression. Excluding `=`
matches the HTML5 unquoted-attribute grammar, but browsers accept it and people
write it. Dropping `=` from the class is safe here: the value still has to
clear `SUBSET_LINK_PATTERN` and is re-escaped on output.
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -80,7 +80,12 @@ public class Utilities {
private static final Pattern CLOSING_A_TAG_PATTERN = Pattern.compile(
"</a>", Pattern.CASE_INSENSITIVE);
private static final Pattern OPENING_A_TAG_PATTERN = Pattern.compile(
- "<a href=.*?>", Pattern.CASE_INSENSITIVE);
+ "<a\\s+href\\s*=.*?>", Pattern.CASE_INSENSITIVE |
Pattern.DOTALL);
+ private static final Pattern A_HREF_PATTERN = Pattern.compile(
+
"<a\\s+href\\s*=\\s*(?:\"([^\"]*)\"|'([^']*)'|([^\\s\"'=<>`]+))\\s*>",
+ Pattern.CASE_INSENSITIVE);
Review Comment:
🐞Claude Issue: **Important:** This pattern is applied with `.matches()`
(line 990), so the opening tag must be *exactly* `<a href=X>`. Any additional
attribute makes the match fail and the href is discarded:
```
<a href="https://example.com/"; target="_blank">x</a> -> <a>x</a>
<a href='https://example.com/' title='site'>x</a> -> <a>x</a>
<a href="https://example.com/"; rel="nofollow">x</a> -> <a>x</a>
```
`title`, `target` and `rel` are common in pasted comment HTML and the link
dies for all of them. Tolerating and discarding trailing attributes is safe
precisely because the output anchor is reconstructed from the validated href
and never echoes the input tag — adding a `(?:\s[^&]*?)?` tail before `\s*>`
recovers all three cases, while `<a href="javascript:alert(1)" onclick="x">`
still degrades to `<a>` via the scheme allowlist.
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -975,23 +980,39 @@ public static String transformToHTMLSubset(String s) {
s = replace(s, CLOSING_LI_TAG_PATTERN, "</li>");
s = replace(s, QUOTE_PATTERN, "\"");
- // HTTP links
+ // Normalize supported links while retaining the surrounding text.
s = replace(s, CLOSING_A_TAG_PATTERN, "</a>");
Review Comment:
🐞Claude Issue: **Important:** `CLOSING_A_TAG_PATTERN` rewrites `</a>`
unconditionally, but the opening tag is only rewritten when
`OPENING_A_TAG_PATTERN` matches. Anchors it does not recognise therefore emit
an unbalanced `</a>`:
```
<a>x</a> -> <a>x</a>
<a class="c" href="https://x/";>y</a> -> <a class="c"
href="https://x/">y</a>
```
This is pre-existing, but the loop is being rewritten here and already has
an "unsupported anchor" output (`<a>`). Emitting that for unmatched opening
tags as well would keep the markup balanced for a couple of lines of change.
--
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]
